From ef3215ab0b8a4388d39b303a2502d78fd8da79ae Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 9 Oct 2015 14:15:25 -0700 Subject: [PATCH] Fix recompiles where overrides changed This commit causes a recompile to be triggered if the overridden version of a build script changed since it was last run. Previously this wasn't tracked very well due to fingerprints not accounting for the right data. There are a few architectural changes here which were made to prepare for this: * The unit of work for "running a build script" is now emitted as dependency regardless of whether a build script is overridden or not. Previously it was omitted if overridden. * This unit of work has 0 dependencies if it is overridden (as we know the output) and otherwise has the normal set of dependencies. * The fingerprint calculation was updated to recognize when a build script execution is overridden and instead consider the overridden value as input to the fingerprint. This means that if the overridden values change they will trigger a recompile. * The "prepare a build script to run" step now emits a noop if the execution of the build script is overridden. After putting all that together, this commit ... Closes #2042 --- src/cargo/ops/cargo_rustc/context.rs | 79 ++++++++++++----------- src/cargo/ops/cargo_rustc/custom_build.rs | 36 +++++++---- src/cargo/ops/cargo_rustc/fingerprint.rs | 15 ++++- tests/test_cargo_compile_custom_build.rs | 75 +++++++++++++++++++++ 4 files changed, 154 insertions(+), 51 deletions(-) diff --git a/src/cargo/ops/cargo_rustc/context.rs b/src/cargo/ops/cargo_rustc/context.rs index 50c44635d..89a2c1a08 100644 --- a/src/cargo/ops/cargo_rustc/context.rs +++ b/src/cargo/ops/cargo_rustc/context.rs @@ -290,7 +290,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { /// for that package. pub fn dep_targets(&self, unit: &Unit<'a>) -> Vec> { if unit.profile.run_custom_build { - return self.dep_run_custom_build(unit, false) + return self.dep_run_custom_build(unit) } else if unit.profile.doc { return self.doc_deps(unit); } @@ -345,12 +345,13 @@ impl<'a, 'cfg> Context<'a, 'cfg> { }) }).collect::>(); - // If a target isn't actually a build script itself, then it depends on - // the build script if there is one. + // If this target is a build script, then what we've collected so far is + // all we need. If this isn't a build script, then it depends on the + // build script if there is one. if unit.target.is_custom_build() { return ret } - ret.extend(self.build_script_if_run(unit, false)); + ret.extend(self.dep_build_script(unit)); // If this target is a binary, test, example, etc, then it depends on // the library of the same package. The call to `resolve.deps` above @@ -376,9 +377,24 @@ impl<'a, 'cfg> Context<'a, 'cfg> { return ret } - pub fn dep_run_custom_build(&self, - unit: &Unit<'a>, - include_overridden: bool) -> Vec> { + /// Returns the dependencies needed to run a build script. + /// + /// The `unit` provided must represent an execution of a build script, and + /// the returned set of units must all be run before `unit` is run. + pub fn dep_run_custom_build(&self, unit: &Unit<'a>) -> Vec> { + // If this build script's execution has been overridden then we don't + // actually depend on anything, we've reached the end of the dependency + // chain as we've got all the info we're gonna get. + let key = (unit.pkg.package_id().clone(), unit.kind); + if self.build_state.outputs.lock().unwrap().contains_key(&key) { + return Vec::new() + } + + // When not overridden, then the dependencies to run a build script are: + // + // 1. Compiling the build script itself + // 2. For each immediate dependency of our package which has a `links` + // key, the execution of that build script. let not_custom_build = unit.pkg.targets().iter().find(|t| { !t.is_custom_build() }).unwrap(); @@ -387,18 +403,16 @@ impl<'a, 'cfg> Context<'a, 'cfg> { profile: &self.profiles.dev, ..*unit }; - let mut ret = self.dep_targets(&tmp).iter().filter_map(|unit| { + self.dep_targets(&tmp).iter().filter_map(|unit| { if !unit.target.linkable() || unit.pkg.manifest().links().is_none() { return None } - self.build_script_if_run(unit, include_overridden) - }).collect::>(); - ret.push(Unit { + self.dep_build_script(unit) + }).chain(Some(Unit { profile: self.build_script_profile(unit.pkg.package_id()), - kind: Kind::Host, + kind: Kind::Host, // build scripts always compiled for the host ..*unit - }); - return ret + })).collect() } /// Returns the dependencies necessary to document a package @@ -442,7 +456,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { } // Be sure to build/run the build script for documented libraries as - ret.extend(self.build_script_if_run(unit, false)); + ret.extend(self.dep_build_script(unit)); // If we document a binary, we need the library available if unit.target.is_bin() { @@ -451,28 +465,21 @@ impl<'a, 'cfg> Context<'a, 'cfg> { return ret } - /// Returns the build script for a package if that build script is actually - /// intended to be run for `kind` as part of this compilation. + /// If a build script is scheduled to be run for the package specified by + /// `unit`, this function will return the unit to run that build script. /// - /// Build scripts are not run if they are overridden by some global - /// configuration. - fn build_script_if_run(&self, unit: &Unit<'a>, - allow_overridden: bool) -> Option> { - let target = match unit.pkg.targets().iter().find(|t| t.is_custom_build()) { - Some(t) => t, - None => return None, - }; - let key = (unit.pkg.package_id().clone(), unit.kind); - if !allow_overridden && - unit.pkg.manifest().links().is_some() && - self.build_state.outputs.lock().unwrap().contains_key(&key) { - return None - } - Some(Unit { - pkg: unit.pkg, - target: target, - profile: &self.profiles.custom_build, - kind: unit.kind, + /// Overriding a build script simply means that the running of the build + /// script itself doesn't have any dependencies, so even in that case a unit + /// of work is still returned. `None` is only returned if the package has no + /// build script. + fn dep_build_script(&self, unit: &Unit<'a>) -> Option> { + unit.pkg.targets().iter().find(|t| t.is_custom_build()).map(|t| { + Unit { + pkg: unit.pkg, + target: t, + profile: &self.profiles.custom_build, + kind: unit.kind, + } }) } diff --git a/src/cargo/ops/cargo_rustc/custom_build.rs b/src/cargo/ops/cargo_rustc/custom_build.rs index fa5d689f7..8431e81c5 100644 --- a/src/cargo/ops/cargo_rustc/custom_build.rs +++ b/src/cargo/ops/cargo_rustc/custom_build.rs @@ -15,7 +15,7 @@ use super::{fingerprint, process, Kind, Context, Unit}; use super::CommandType; /// Contains the parsed output of a custom build script. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Hash)] pub struct BuildOutput { /// Paths to pass to rustc with the `-L` flag pub library_paths: Vec, @@ -49,6 +49,23 @@ pub fn prepare(cx: &mut Context, unit: &Unit) -> CargoResult<(Work, Work, Freshness)> { let _p = profile::start(format!("build script prepare: {}/{}", unit.pkg, unit.target.name())); + let key = (unit.pkg.package_id().clone(), unit.kind); + let overridden = cx.build_state.outputs.lock().unwrap().contains_key(&key); + let (work_dirty, work_fresh) = if overridden { + (Work::new(|_| Ok(())), Work::new(|_| Ok(()))) + } else { + try!(build_work(cx, unit)) + }; + + // Now that we've prep'd our work, build the work needed to manage the + // fingerprint and then start returning that upwards. + let (freshness, dirty, fresh) = + try!(fingerprint::prepare_build_cmd(cx, unit)); + + Ok((work_dirty.then(dirty), work_fresh.then(fresh), freshness)) +} + +fn build_work(cx: &mut Context, unit: &Unit) -> CargoResult<(Work, Work)> { let (script_output, build_output) = { (cx.layout(unit.pkg, Kind::Host).build(unit.pkg), cx.layout(unit.pkg, unit.kind).build_out(unit.pkg)) @@ -90,7 +107,7 @@ pub fn prepare(cx: &mut Context, unit: &Unit) // This information will be used at build-time later on to figure out which // sorts of variables need to be discovered at that time. let lib_deps = { - cx.dep_run_custom_build(unit, true).iter().filter_map(|unit| { + cx.dep_run_custom_build(unit).iter().filter_map(|unit| { if unit.profile.run_custom_build { Some((unit.pkg.manifest().links().unwrap().to_string(), unit.pkg.package_id().clone())) @@ -117,7 +134,7 @@ pub fn prepare(cx: &mut Context, unit: &Unit) // // Note that this has to do some extra work just before running the command // to determine extra environment variables and such. - let work = Work::new(move |desc_tx| { + let dirty = Work::new(move |desc_tx| { // Make sure that OUT_DIR exists. // // If we have an old build directory, then just move it into place, @@ -181,15 +198,6 @@ pub fn prepare(cx: &mut Context, unit: &Unit) // Now that we've prepared our work-to-do, we need to prepare the fresh work // itself to run when we actually end up just discarding what we calculated // above. - // - // Note that the freshness calculation here is the build_cmd freshness, not - // target specific freshness. This is because we don't actually know what - // the inputs are to this command! - // - // Also note that a fresh build command needs to - let (freshness, dirty, fresh) = - try!(fingerprint::prepare_build_cmd(cx, unit)); - let dirty = work.then(dirty); let fresh = Work::new(move |_tx| { let (id, pkg_name, build_state, build_output) = all; let contents = try!(paths::read(&build_output.parent().unwrap() @@ -197,9 +205,9 @@ pub fn prepare(cx: &mut Context, unit: &Unit) let output = try!(BuildOutput::parse(&contents, &pkg_name)); build_state.insert(id, kind, output); Ok(()) - }).then(fresh); + }); - Ok((dirty, fresh, freshness)) + Ok((dirty, fresh)) } impl BuildState { diff --git a/src/cargo/ops/cargo_rustc/fingerprint.rs b/src/cargo/ops/cargo_rustc/fingerprint.rs index 817e7894a..945b42f23 100644 --- a/src/cargo/ops/cargo_rustc/fingerprint.rs +++ b/src/cargo/ops/cargo_rustc/fingerprint.rs @@ -378,7 +378,20 @@ pub fn prepare_build_cmd(cx: &mut Context, unit: &Unit) debug!("fingerprint at: {}", loc.display()); - let new_fingerprint = try!(calculate_pkg_fingerprint(cx, unit.pkg)); + // If this build script execution has been overridden, then the fingerprint + // is just a hash of what it was overridden with. Otherwise the fingerprint + // is that of the entire package itself as we just consider everything as + // input to the build script. + let new_fingerprint = { + let state = cx.build_state.outputs.lock().unwrap(); + match state.get(&(unit.pkg.package_id().clone(), unit.kind)) { + Some(output) => { + format!("overridden build state with hash: {}", + util::hash_u64(output)) + } + None => try!(calculate_pkg_fingerprint(cx, unit.pkg)), + } + }; let new_fingerprint = Arc::new(Fingerprint { rustc: 0, target: 0, diff --git a/tests/test_cargo_compile_custom_build.rs b/tests/test_cargo_compile_custom_build.rs index 3f2788d06..a28bd4633 100644 --- a/tests/test_cargo_compile_custom_build.rs +++ b/tests/test_cargo_compile_custom_build.rs @@ -1411,3 +1411,78 @@ test!(diamond_passes_args_only_once { {running} `[..]rlib -L native=test` ", compiling = COMPILING, running = RUNNING))); }); + +test!(adding_an_override_invalidates { + let target = ::rustc_host(); + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.5.0" + authors = [] + links = "foo" + build = "build.rs" + "#) + .file("src/lib.rs", "") + .file(".cargo/config", "") + .file("build.rs", r#" + fn main() { + println!("cargo:rustc-link-search=native=foo"); + } + "#); + + assert_that(p.cargo_process("build").arg("-v"), + execs().with_status(0).with_stdout(&format!("\ +{compiling} foo v0.5.0 ([..] +{running} `rustc [..]` +{running} `[..]` +{running} `rustc [..] -L native=foo` +", compiling = COMPILING, running = RUNNING))); + + File::create(p.root().join(".cargo/config")).unwrap().write_all(format!(" + [target.{}.foo] + rustc-link-search = [\"native=bar\"] + ", target).as_bytes()).unwrap(); + + assert_that(p.cargo("build").arg("-v"), + execs().with_status(0).with_stdout(&format!("\ +{compiling} foo v0.5.0 ([..] +{running} `rustc [..] -L native=bar` +", compiling = COMPILING, running = RUNNING))); +}); + +test!(changing_an_override_invalidates { + let target = ::rustc_host(); + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.5.0" + authors = [] + links = "foo" + build = "build.rs" + "#) + .file("src/lib.rs", "") + .file(".cargo/config", &format!(" + [target.{}.foo] + rustc-link-search = [\"native=foo\"] + ", target)) + .file("build.rs", ""); + + assert_that(p.cargo_process("build").arg("-v"), + execs().with_status(0).with_stdout(&format!("\ +{compiling} foo v0.5.0 ([..] +{running} `rustc [..] -L native=foo` +", compiling = COMPILING, running = RUNNING))); + + File::create(p.root().join(".cargo/config")).unwrap().write_all(format!(" + [target.{}.foo] + rustc-link-search = [\"native=bar\"] + ", target).as_bytes()).unwrap(); + + assert_that(p.cargo("build").arg("-v"), + execs().with_status(0).with_stdout(&format!("\ +{compiling} foo v0.5.0 ([..] +{running} `rustc [..] -L native=bar` +", compiling = COMPILING, running = RUNNING))); +}); -- 2.30.2